Skip to content

fix(storage): write userData JSON files owner-only - #5

Merged
voidstackloop merged 1 commit into
voidstackloop:mainfrom
ConsultingFuture4200:fix/secrets-file-permissions
Jul 25, 2026
Merged

fix(storage): write userData JSON files owner-only#5
voidstackloop merged 1 commit into
voidstackloop:mainfrom
ConsultingFuture4200:fix/secrets-file-permissions

Conversation

@ConsultingFuture4200

Copy link
Copy Markdown
Contributor

Problem

writeJson backs secrets.json (provider API keys), sessions.json, settings.json and projects.json. It wrote with no mode, so the files landed at the process umask — 0644 normally, 0664 under the umask 002 several distributions ship.

That leaves the contents readable to anything that reaches the file: another account on a shared machine, a backup or sync tool, an archive unpacked somewhere else. On a single-user desktop the userData directory is usually restrictive enough to cover it — but a stored credential shouldn't depend on its parent directory's mode, and secrets-store.ts falls back to storing keys unencrypted when safeStorage.isEncryptionAvailable() is false, which is common on Linux without a keyring.

There was also no way for a user to fix it themselves: writeJson writes a temp file and renames over the target, so a hand-applied chmod 600 was silently reset by the next write.

Fix

The mode goes on the temp file. The rename replaces the destination inode, so the temp file's mode is the mode the stored file ends up with. Chmod'ing the destination instead would leave a window where the contents are readable and be undone on the next write.

Because writeFileSync only applies mode when it creates the file, the temp path is removed first rather than chmod'd afterwards. That handles a stale temp file left by an interrupted write under the same pid, without adding a syscall between the write and the rename that could fail and lose the data.

Existing files are tightened on first read, once per path per run. Without that this would only protect fresh installs: a key set once and never changed is only ever read, so a write-side fix would never reach the installs that actually have the problem. That step is best-effort — it logs and continues rather than throwing, so unusual ownership can't stop the app reading its own data.

No behaviour change on Windows, where the mode has no meaning; those tests are skipped.

Tests

  • A written file is 0600.
  • It is still 0600 after a rewrite.
  • It is 0600 even when a stale temp file with a permissive mode is in the way.
  • An existing 0644 file is tightened when it is read, and still parses.

Control experiment: the first three fail on main and pass with the patch.

I checked the corrupted-file backup path is unaffected — copyFileSync clones the source mode, so a 0600 file yields a 0600 .corrupted-* backup.

npx tsc -p tsconfig.json --noEmit clean; npm test 231 passed.

writeJson backs secrets.json (provider API keys), sessions.json,
settings.json and projects.json. It wrote with no mode, so the files landed
at the process umask — 0644 normally, 0664 under the umask 002 several
distributions ship — leaving their contents readable to anything that
reaches them: another account on a shared machine, a backup or sync tool,
an archive unpacked elsewhere. The userData directory is usually
restrictive enough to cover that on a single-user desktop, but a stored
credential shouldn't depend on its parent directory's mode.

The mode has to go on the temp file rather than the destination: writeJson
writes to a temp path and renames over the target, so the destination inode
is replaced on every write and takes the temp file's mode with it. Chmod'ing
the destination instead would leave a window where the contents are
readable and be undone by the next write — which also means a user who
chmod 600'd the file by hand had it silently reset.

Because writeFileSync only applies `mode` when it creates the file, the temp
path is removed first rather than chmod'd afterwards; that keeps a stale
temp file from a crashed write from carrying its old mode through, without
adding a syscall that could fail between the write and the rename and lose
the data.

Existing files are tightened on first read, once per path per run — a key
set once and never changed is only ever read, so fixing this on write alone
would never reach the installs that already have the problem. That step is
best-effort and logs rather than throwing. No behaviour change on Windows,
where the mode has no meaning and the tests are skipped.
@voidstackloop
voidstackloop merged commit 72435a1 into voidstackloop:main Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants